chore(scripts): refactor update-electron to general purpose update-dependencies; replace usage in CI #7071
Conversation
| // NB: We're always trying to update to latest major, this usually implies | ||
| // breaking changes, but those rarely affect us. If it becomes a problem, we | ||
| // can always change this code to lock it to whatever major version of | ||
| // electron compass is currently at | ||
| 'electron', |
There was a problem hiding this comment.
This is a "breaking change" compared to what the old script was doing, manually trying this update locally it doesn't seem to break anything and as mentioned I do think we rarely affected by those, so it's probably okay to try to always update to latest, but tell me if you have thoughts on that.
gribnoysup
added
the
no-title-validation
|
|
||
| const UPDATE_CONFIGS = require('./update-dependencies-config'); | ||
|
|
||
| async function hoistSharedDependencies(newVersions) { |
There was a problem hiding this comment.
To be very clear, this flow is definitely sub optimal, we run install three (!) times, but it was also the only way we could consistently make npm do what we want instead of whatever it decides to do wrt shared dependencies and their current location in the dependency tree and debugging npm behavior on a pretty complex dependency tree is really really hard and I'm not sure it's worth it
There was a problem hiding this comment.
When we decide to update other deps using this scripts, they will also be hoisted at the top. And then running locally npm bootstrap, would it create updates in lock file?
There was a problem hiding this comment.
I tested it with a few different dependencies and lock stays stable (meaning nothing is unexpectedly unhoisted) after running script a couple of times with different deps, but to be honest I don't know how to validate that this will never happen at all, I lost any trust in npm internal algorithms at this point 😆
…pendencies; replace usage in CI
gribnoysup
force-pushed
the
update-dependencies-script
branch
from
9f3aacd to
a92db77
Compare
| console.log('Successfully updated dependencies'); | ||
| } | ||
|
|
||
| main(); |
There was a problem hiding this comment.
Would it make sense to update this to typescript? If it's possible that would be nice! Not a blocker.
There was a problem hiding this comment.
Hmm, will need to add ts-register and allow cli.js to run typescript first, I'll take a stab as a follow-up
We disabled autoprs from
dependabotrecently because the tool doesn't really work in the monorepo (this is probably just more related to npm behaving weirdly often and not being able to do group version bumps well, but all dependabot PRs had various issues in them making them practically useless), but there's still a need for us to have some automation around version update especially because, as mentioned, doing it with npm is a pain sometimes.This patch repurposes existing update-electron script to allow for it to work as a general purpose cli script to consistently update multiple packages to a certain version across our monorepo going through steps that we found work well and make sure that dependency tree is consistent.
For starters I'm replacing existing update electron and update eslint tasks to use the new script, in the future we can add other groups described in the _dependabot file to use it, I'm leaving this out for now, there's a ticket that we have to track this work.